fix(pin-corpus): key retirement on site identity, not literal alone (#1006) - #1044
Conversation
Retirement was keyed by literal while revival authorization is keyed by site, so a retained boundary pin sharing a literal with a redundant_retire'd twin at a different site inherited an unsatisfiable gate finding with no exit (issue #1006, surfaced by PR #1005). Key retirement on (source_file, helper, literal, resolved_target) — the site fields RevivalAuthorization already carries. GuardSite now carries a classifier-equivalent resolved_target_token (repo-relative path, runtime bundle placeholder, or None) so a live site's key matches the manifest's resolved_target cell; the manifest's superseded .devflow/ target spelling is normalized to .prflow/ for the comparison. The literal-scoped adjudication ledger lookups stay literal-keyed. Empirically: the old literal-keying poisoned 62 live retained pins; site- keying poisons 0, while retirement stays effective (129 retired site keys). 4-tuple keying resolves all 7 retire/retain manifest collisions the 3-tuple would leave. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- GuardSite.retirement_key() as the single site->key mapping (removes the duplicated 4-tuple construction across scan_changed_sources' two loops and the redundant None guard). - Extract _repo_relative_or_none, shared by _resolved_target_token and _normalized_revival_authorization (was a duplicated abspath/commonpath/relpath). - Document the runtime-target sentinel as a coupled invariant with pin-corpus-classifier.py, and the deliberate out-of-repo fail-toward-not-matched divergence from the classifier's raw-path behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fix) Review-and-fix loop findings (all test-coverage, no correctness defects): - test_resolved_target_token_matches_the_classifier_three_shapes: covers the file/bundle/None token shapes, the .devflow->.prflow normalization round-trip (75 real manifest targets depend on it), and the DEVFLOW-filename non-goal. - test_malformed_retirement_manifest_site_fields_fail_closed: drives the new source_file/resolved_target JSON-parse fail-closed arms. - Docstring: note the vendored sub-path is deliberately out of normalization scope; fix a doc stutter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Requesting review. Implements #1006: pin retirement now keys on site identity ( This is a security-adjacent guard, so the question that matters is whether it narrowed correctly or simply went quiet. I verified the negative control exists and discriminates — a parameterized pair with opposite expectations: Please pressure-test exactly that boundary:
/prflow:review |
![prflow-reviewer[bot]](https://avatars.githubusercontent.com/u/223979750?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: APPROVE with notes (correct, well-scoped keying fix; one non-blocking test-hardening suggestion)
Issue Compliance
Reviewed against issue #1006: Pin retirement is keyed by literal while revival authorization is keyed by site — criteria from the issue body's Acceptance Criteria; scope unchanged. Requirement-based checklist items are included in the verification results below.
All acceptance criteria are satisfied:
- Site-keying implemented —
_site_retirement_key(source_file, helper, literal, resolved_target)replaces_literal_adjudication_keyfor retirement, applied symmetrically on the manifest-read side (_strict_retirement_manifest_literals) and the live-site side (GuardSite.retirement_key()). - Different-site pin no longer reported / own-site still reported — driven end-to-end by
test_retirement_is_scoped_to_the_retired_site(0 vs 1 findings, with the own-site case as an explicit negative control). - RETAIN/RETIRE per-site conflict resolved —
test_conflicting_retain_and_retire_dispositions_resolve_per_site. - Frozen manifests read, not rewritten — the
.devflow/→.prflow/skew is handled at key-build time by_normalize_retirement_target(applied to both comparison sides); manifest bytes untouched. - Tests updated with rationale, not deleted — each changed expectation carries an accurate issue-#1006 rationale comment, verified against the real frozen manifest rows.
Verification Checklist Results
No FAIL, no INCONCLUSIVE. Engine-self-modifying diff → full checklist scope. CI ground truth for the reviewed HEAD (28a5c14) reports lib + python tests: success and all shards success; the four new/changed tests were additionally re-run in this review's environment and pass. AC7 (lib/test/run.sh zero failures/zero skips) is discharged by the green suite.
Code Review Findings
🟡 Suggestion / Minor
- The classifier-equivalence coupled invariant has no binding test.
test_resolved_target_token_matches_the_classifier_three_shapesasserts_resolved_target_token/_site_retirement_keyagainst hardcoded literals ("docs/x.md","/__pin_corpus_runtime__/CI_BUNDLE"), andtest_pin_corpus_classifier.pyindependently pins the same sentinel to its own literals. No test imports both modules and assertslint._resolved_target_token(x) == classifier._portable_target(x)/recover_override_names. A one-sided edit to the classifier's sentinel or its relativization would keep both scripts' own tests green while silently breaking every runtime-bundle/file retirement key match — the exact silent no-op the code's own coupled-invariant comment (pin-corpus-lint.py:1670-1673) warns about. Recommend one cross-module equality test (the harness already loads one module into the other's tests viaimportlib). (raised by 2/5 agents) — Non-blocking: the coupling is disclosed via a coupled-invariant comment per CLAUDE.md's discipline, which the repo accepts; a binding test would be strictly stronger. - Minor fail-closed coverage gap:
test_malformed_retirement_manifest_site_fields_fail_closedexercises the source-sideJSONDecodeErrorand the non-string source/target arms, but not an invalid-JSONresolved_targetcell (thejson.loads(row[target_index])decode-error arm). Bundle-target retirement and the.devflow/→.prflow/normalization are covered only at the unit-token level, not end-to-end throughscan_changed_sources. Low risk —scan_changed_sourcesconsumesretirement_key()uniformly. (raised by 1/5 agents)
ℹ️ Informational
_repo_relative_or_nonedocstring calls itself "the one copy of the abspath/commonpath/relpath computation the file otherwise repeats" while the same sentence points at a genuine second (raising) copy,_relative_target_path. Behavior-inert wording nit; suggest "the non-raising copy …_relative_target_pathis the raising variant." (raised by 1/5 agents)
Verdict Criteria
- No verification checklist FAIL / INCONCLUSIVE.
- No self-contradicting-diff finding — the comment/docstring truthfulness sweep confirmed every load-bearing claim accurate against HEAD and the real manifests.
- No review-agent finding at or above the
criticalverdict threshold; only Suggestion/Informational findings present → APPROVE with notes.
over-grade annotation: no finding flagged
truthfulness sweep: no finding promoted
intra-diff contradiction scan: no contradiction found
Scope verified beyond code-reading: the change is a strict narrowing of retirement identity (site ⊂ literal), so it cannot make retirement over-broad; the documented out-of-repo/unresolvable→None direction is fail-toward-not-matched (routes through ordinary policy, still reports an undeclared pin), and the _normalized_revival_authorization extraction of _repo_relative_or_none was verified behavior-preserving.
|
Review verdict on Beyond the review, I verified the property that distinguishes this fix from a hole — the negative control discriminates rather than going quiet: Opposite expectations on the same literal, both driven. A gate that simply stopped firing would fail the own-site arm. The two 🟡 suggestions are about future drift rather than current correctness — the classifier-equivalence invariant is asserted against hardcoded literals rather than bound to the classifier, and the invalid-JSON Merging. |
1 participant
Closes #1006.
Summary
Pin retirement was keyed by literal while revival authorization is keyed by site, so a retained
boundarypin sharing a literal with aredundant_retired twin at a different site inherited an unsatisfiable gate finding with no exit (surfaced by PR #1005).Retirement now keys on site identity —
(source_file, helper, literal, resolved_target), the fieldsRevivalAuthorizationalready carries.GuardSitegains a classifier-equivalentresolved_target_token(repo-relative path,/__pin_corpus_runtime__/<var>bundle placeholder, or None) so a live site's key matches the manifest'sresolved_targetcell; the frozen.devflow/target spelling normalizes to.prflow/for the comparison. The literal-scoped adjudication ledger lookups stay literal-keyed.Why 4-tuple (target included)
An executed scan of the frozen manifests shows the 3-tuple
(source_file, helper, literal)leaves 7 retire/retain collisions (6assert_pin_uniquepairs differing only by target, plus one bundle case); addingresolved_targetleaves zero. This is what satisfies AC4 (the issue's AC1 shorthand omits target; AC4 requires it — noted as an issue-accuracy reflection).Evidence (driven end-to-end, not code reading)
run.sh:2673); site-keying poisons 0, while retirement stays effective (129 retired site keys).pin-corpus-lint.py mutation-routing-worktree .returns rc 0 over this branch.Tests
test_retirement_is_scoped_to_the_retired_site— AC2 (different-site not reported) + AC3 negative control (own-site still reported).test_conflicting_retain_and_retire_dispositions_resolve_per_site— AC4.test_resolved_target_token_matches_the_classifier_three_shapes— the file/bundle/None token shapes plus the.devflow/→.prflow/normalization round-trip and theDEVFLOW-filename non-goal (added in the review-and-fix pass).test_malformed_retirement_manifest_site_fields_fail_closed— the newsource_file/resolved_targetJSON-parse fail-closed arms (added in the review-and-fix pass).test_authorized_retired_revival_cannot_launder_committed_prose_target,test_a_retired_literal_keeps_the_pre_948_revival_contract,test_convert_presence_is_not_a_retired_wording_disposition, the count-helper tests, and the shared fixture (site-aware).Frozen manifests are read, never rewritten. No changeset:
lib/test/is stripped from the vendored plugin (internal test/CI infrastructure).🤖 Generated with Claude Code